Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Elastic-Charts to 13.0.0 #2381

Merged
merged 14 commits into from
Sep 30, 2019
Merged

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 26, 2019

This removed the necessity of specifying the grid styles outside of the theme object and a simpler way to get rid of the axes label ticks. I also added a callout for Kibana engineers over how to use the Kibana plugin for getting the EUI theme.

image

Also

This also contains a fix where EuiCodeBlock's text size wasn't changing based on the fontSize prop if they occurred inside a EuiText wrapper. I ensured proper inheritance for the component itself while reducing specificity of the EuiText sizing class by removing the chained selectors.

Checklist

  • Checked in dark mode
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
  • [ ] Props have proper autodocs
  • Added documentation examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos
Copy link
Contributor Author

cchaos commented Sep 26, 2019

Are dependency upgrades considered breaking if the dependency had breaking changes?

<Settings theme={euiTheme} />`}
</EuiCodeBlock>
<EuiCallOut title="Kibana engineers" iconType="logoKibana">
<p>
We provide a plugin utility for ease of pulling in the correct
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only commentary would be that this shouldn't link to a pull request, which is essentially a snapshot from a previous time. You might want to consider adding a readme (in absence of proper dev docs there) to your plugin, then linking to it from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think I was just being lazy. I'll go create a readme...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on approval of elastic/kibana#46736

@chandlerprall
Copy link
Contributor

Are dependency upgrades considered breaking if the dependency had breaking changes?

Depends what kind of dependency it is. In this case the dependency is listed in devDependencies and only affects direct EUI installs as opposed to other projects listing EUI as in their dependencies. This gets a little weird because of _src/themes/charts/themes.ts _, but we don't bundle that with the main distribution so it isn't a required dependency.

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code and docs changes LGTM

@thompsongl
Copy link
Contributor

Just curious what the plan is for future updates. Wait for a bump in Kibana and react to it? Watch the release notes and upgrade when new/breaking changes are introduced? Catch every release?

@cchaos
Copy link
Contributor Author

cchaos commented Sep 26, 2019

Ideally, the elastic-charts team would not take care of these larger upgrades in Kibana. This is definitely where I'm worried about versioning issues being incompatible. Because even though the theme file isn't distributed, when we update EUI in Kibana, but Kibana isn't on the latest Charts there will be a TS conflict in that theme file.

I'm open to suggestions.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 26, 2019

I updated my comment above as I meant to say the charts team would ideally take care of updates.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 30, 2019

I updated the Kibana callout to just reference a link to the Readme (removing the code portion). This way we don't have to maintain that code in two places.

@cchaos cchaos requested a review from snide September 30, 2019 19:24
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY

@cchaos cchaos merged commit dc1e7f8 into elastic:master Sep 30, 2019
@cchaos cchaos deleted the update-charts-12.0.0 branch September 30, 2019 21:12
snide pushed a commit to snide/eui that referenced this pull request Oct 10, 2019
* Update to Charts 13.0.0

* Move grid line styles into the theme and remove from the examples

* Adding scales in even though they’re the default

* Fix inheritence of code block font sizes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants